S3Client: don't double-deref path when a method fails after blob construction#30419
S3Client: don't double-deref path when a method fails after blob construction#30419robobun wants to merge 4 commits into
Conversation
constructS3FileInternalStore / constructS3FileWithS3CredentialsAndOptions pass the PathLike to Blob.Store.initS3 which calls toThreadSafe(), transferring ownership of the underlying WTFStringImpl to the store. When getPresignUrlFrom subsequently fails (e.g. missing credentials), both `defer blob.deinit()` and the outer `errdefer path.deinit()` ran, deref'ing the same string twice and tripping the hasAtLeastOneRef assert (or corrupting the refcount in release builds). Clear the caller's path handle once ownership has moved to the blob, matching the pattern in Blob.findOrCreateFileFromPath.
|
Updated 2:07 AM PT - May 9th, 2026
❌ @robobun, your commit a044067 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 30419That installs a local version of the PR into your bun-30419 --bun |
WalkthroughDefaults presign/path branches to an empty PathString after consuming options, centralizes blob content-type assignment in S3File constructors via setBlobContentType, removes local store errdefer cleanup patterns, and adds a test for missing S3 credentials. ChangesS3 Presign Path Handling and Constructors Refactor
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webcore/S3File.zig`:
- Around line 85-89: The path ownership must be cleared at the moment the helper
takes ownership to avoid double-free if the helper later throws; modify
constructS3FileInternalStore (or create an overload that accepts a mutable
PathLike) so it blanks the incoming PathString/PathLike immediately when
Blob.Store.initS3 takes ownership (i.e., right after the transfer) rather than
relying on the caller to clear path_or_blob; ensure the helper returns the
initialized blob with the original path cleared (or returns an error without
double-deref) so callers of constructS3FileInternalStore and callers of
getPresignUrlFrom won't end up deinitializing the same PathString twice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a44a4a09-e690-4628-a870-88feaa8496d7
📒 Files selected for processing (3)
src/runtime/webcore/S3Client.zigsrc/runtime/webcore/S3File.zigtest/js/bun/s3/s3-presign-missing-credentials.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
getCredentialsWithOptions already reads and validates options.type into aws_options.content_type. Re-reading it after initS3 was the only fallible operation past the point where the store takes ownership of the path; a getter that throws on the second access would trip the same double-deref as the presign case. Use the already-parsed slice and drop the now-dead errdefer store.deinit(). As a side effect options.type is read once instead of twice.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
write() was hitting the same double-deref when data is null/undefined: Blob.writeFileInternal throws synchronously before creating a promise, so both defer blob.detach() and errdefer path.deinit() ran. exists/size/ stat/unlink have fallible tails too. Apply the same post-transfer clear everywhere the helper hands the path to the store.
robobun
changed the title
There was a problem hiding this comment.
LGTM — c8149ec addresses my earlier feedback by applying the post-transfer path clear to all siblings in both files, and the helper refactor makes the fail-before-transfer-or-succeed invariant hold.
Extended reasoning...
Overview
This PR fixes a double-deref crash in S3Client/S3File methods (presign, write, exists, size, stat, unlink) when an operation fails synchronously after the blob store has taken ownership of the path's WTFStringImpl. The fix is two-pronged: (1) clear the caller's path handle to .{ .string = bun.PathString.empty } immediately after the construction helper returns, so the outer errdefer path.deinit() becomes a no-op; (2) refactor the construction helpers to reuse aws_options.content_type (already parsed by getCredentialsWithOptions) instead of re-reading options.type from JS, which removes the only fallible operation after initS3 and lets the errdefer store.deinit() be dropped. A regression test spawns a subprocess with credentials cleared and asserts the expected error codes with no stderr.
Follow-up on prior review
My earlier inline comment flagged that write (and the other siblings) could throw synchronously after blob construction, hitting the same double-deref. The author confirmed and applied the same one-line clear to all six methods in both S3Client.zig (instance) and S3File.zig (static) in c8149ec, and extended the test to cover write(path, null). I verified the diff: all 12 call sites now clear the path immediately after the helper returns. The author's note that file is safe (toJS infallible) and listObjects already passes .string = empty checks out.
Security risks
None. This is a memory-safety fix (refcount underflow / use-after-free on a WTF string) — it reduces attack surface rather than adding any. No auth, crypto, or input-validation logic is touched.
Level of scrutiny
Moderate. This is refcount/lifetime management in the runtime, which is subtle territory, but the fix follows an established pattern (Blob.findOrCreateFileFromPath does the same path-clear after ownership transfer) and is mechanically applied. I verified that initWithStore returns Blob (not an error union) and setBlobContentType returns void with only bun.handleOom (abort-on-OOM), so the helper's new contract — fail before initS3, or succeed with nothing fallible after — holds. The aws_options.content_type slice is only read (copied or mapped to a static MIME entry) before defer aws_options.deinit() runs, so no lifetime issue there.
Other factors
CodeRabbit's earlier concern (clearing at the ownership-transfer point) was addressed by making the helper infallible post-initS3 rather than threading *PathLike through ~20 call sites — a cleaner approach. The two CI failures (s3-storage-class.test.ts on a single macOS shard, test-http-should-emit-close-when-connection-is-aborted.ts timeout on Windows) are unrelated to this change. The duplicate-PR bot lists several competing fixes for the same fingerprint; that's a merge-coordination question, not a code-correctness one.
|
CI status: the remaining failures are pre-existing flakes unrelated to this change, present on recently-merged PRs as well.
The Already re-rolled once (a044067); not retriggering again. |
|
Superseded by #30495 (same fix against current main, minimal diff, deterministic test). |
1 participant
What
Fixes a crash in
S3Clientmethods that take a path (presign,write,exists,size,stat,unlink— both static and instance) when the operation fails synchronously after the blob store is constructed.Why
constructS3FileInternalStore/constructS3FileWithS3CredentialsAndOptionshand thePathLiketoBlob.Store.initS3, which callstoThreadSafe()— a transfer of ownership of the underlyingWTFStringImplto the store (no extra ref is taken despite the misleading comment). The caller'spathhandle still aliases the same impl.When a subsequent operation in the caller returns a Zig error (
getPresignUrlFrom→error.MissingCredentials,Blob.writeFileInternal→throwInvalidArgumentson null data, etc.), bothdefer blob.deinit()and the outererrdefer path.deinit()run, deref'ing the same string twice. In debug this tripshasAtLeastOneRef(); in release it aborts or corrupts the refcount.There was also a secondary instance inside the helpers themselves: after
initS3, they re-readoptions.typefrom JS, so a getter that throws on the second access would hit the same double-deref viaerrdefer store.deinit().How
.{ .string = bun.PathString.empty }once ownership has moved to the blob, so theerrdeferbecomes a no-op (.stringvariant ofPathLike.deinitdoes nothing). Matches the existing pattern inBlob.findOrCreateFileFromPath. Applied topresign/write/exists/size/stat/unlinkin bothS3Client.zigandS3File.zig.content_typealready parsed bygetCredentialsWithOptionsinstead of re-readingoptions.type, so nothing fallible runs afterinitS3. As a side effectoptions.typeis read once instead of twice.Found by Fuzzilli. Fingerprint
434aafd9d16f9f0a.